Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

adjusted dockerfile so that build args can be provided to switch the … #8420

Merged
merged 2 commits into from
Dec 19, 2024
Merged

adjusted dockerfile so that build args can be provided to switch the … #8420

merged 2 commits into from
Dec 19, 2024

Conversation

chrisdonlan
Copy link
Contributor

Closes #8419

Change Description

I added the following build args to generalize the Dockerfile to debian::bookworm.

  • BUILD_REPO
  • BUILD_TAG
  • BUILD_PACKAGES
  • IMAGE_REPO
  • IMAGE_TAG
  • IMAGE_PACKAGES
  • ADD_PACKAGES

Background

I mentioned this in LakeFS' help channel in slack, and was recommended to create this issue and make a PR. This is no problem, and I went ahead and generalized the Dockerfile so that we can toggle our base repo, architecture, and so on via our CI/CD build variables.

Bug Fix

If this PR is a bug fix, please let us know about:

  1. Problem - The reason for opening the bug
  2. Root cause - Discovered root cause after investigation
  3. Solution - How the bug was fixed

New Feature

The base docker image can now be built with an arbitrary linux base image, e.g. debian:bookworm:

docker build --build-arg BUILD_REPO=golang --build-arg BUILD_TAG=1.22.6-bookworm --build-arg IMAGE_REPO=debian --build-arg IMAGE_TAG=bookworm --build-arg BUILD_PACKAGES="build-essential ca-certificates" --build-arg IMAGE_PACKAGES=ca-certificates --build-arg ADD_PACKAGES="apt-get update && apt-get install -y" .

Testing Details

I built the image.

Breaking Change?

It doesn't break anything, per say. However, I ran into a bug that I saw on both the unchanged master branch and my updated branch. Namely, I saw the following go error when building on an arm64 mac w/ colima in x86_64 emulation mode via qemu:

 => [build 6/6] COPY . ./                                                                                                                                                                                                                                                                                                                                            1.3s 
 => ERROR [build-lakefs 1/1] RUN --mount=type=cache,target=/root/.cache/go-build     --mount=type=cache,target=/go/pkg     GOOS=linux GOARCH=amd64     go build -ldflags "-X github.com/treeverse/lakefs/pkg/version.Version=dev" -o lakefs ./cmd/lakefs                                                                                                             2.6s 
 => CANCELED [build-lakectl 1/1] RUN --mount=type=cache,target=/root/.cache/go-build     --mount=type=cache,target=/go/pkg     GOOS=linux GOARCH=amd64     go build -ldflags "-X github.com/treeverse/lakefs/pkg/version.Version=dev" -o lakectl ./cmd/lakectl                                                                                                       2.8s 
------                                                                                                                                                                                                                                                                                                                                                                    
 > [build-lakefs 1/1] RUN --mount=type=cache,target=/root/.cache/go-build     --mount=type=cache,target=/go/pkg     GOOS=linux GOARCH=amd64     go build -ldflags "-X github.com/treeverse/lakefs/pkg/version.Version=dev" -o lakefs ./cmd/lakefs:                                                                                                                        
2.397 pkg/authentication/service.go:12:2: no required module provides package github.com/treeverse/lakefs/pkg/authentication/apiclient; to add it:                                                                                                                                                                                                                        
2.397   go get github.com/treeverse/lakefs/pkg/authentication/apiclient
2.397 webui/content.go:7:12: pattern dist: no matching files found

I assume this is because of my setup, and I hope that we'll still be able to build the image in our CI/CD.

Additional info

Contact Details

[email protected]

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

Donlan, Christopher seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@chrisdonlan
Copy link
Contributor Author

I've signed the CLA.

@arielshaqed arielshaqed self-requested a review December 15, 2024 10:40
@arielshaqed arielshaqed added infrastructure build, deploy and release processes contributor include-changelog PR description should be included in next release changelog area/ci labels Dec 15, 2024
@arielshaqed arielshaqed mentioned this pull request Dec 15, 2024
Draft
arielshaqed
arielshaqed reviewed Dec 16, 2024
View reviewed changes
Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this contribution will be great for our more advanced users! I want to pull this, of course.

  • Your CLA is still showing red. This unfortunately happens occasionally. Can you attach a link to somewhere on the CLA Assistant website that shows you have signed it? I am really sorry about this hassle, but this is an unfortunate fact of life with OSS-licensed code.

    I will try to see whether I can kick it into motion in some way on my side.

  • I ran Esti (integration tests) on this PR, all is green.

  • So other than virtual paperwork, I think we're good to go!

@chrisdonlan
Copy link
Contributor Author

I made some changes...will come back to this likely tmrw.

@chrisdonlan
Copy link
Contributor Author

https://cla-assistant.io/treeverse/lakeFS?pullRequest=8420

Screenshot 2024-12-19 at 12 07 53 PM

@chrisdonlan
Copy link
Contributor Author

@arielshaqed updated w/ CLA screen shot. I've given the CLA access to my account, officially, and see it in my doc table.

@arielshaqed arielshaqed mentioned this pull request Dec 19, 2024
Draft
@arielshaqed
Copy link
Contributor

@arielshaqed updated w/ CLA screen shot. I've given the CLA access to my account, officially, and see it in my doc table.

Thanks, @chrisdonlan ! I'm ensuring that we're allowed to replace CLA Assistant with common sense, and will pull if I can or update if we need more help. Really sorry about having to drag this.

@arielshaqed
Copy link
Contributor

@arielshaqed updated w/ CLA screen shot. I've given the CLA access to my account, officially, and see it in my doc table.

Thanks, @chrisdonlan ! I'm ensuring that we're allowed to replace CLA Assistant with common sense, and will pull if I can or update if we need more help. Really sorry about having to drag this.

Okay! Thanks for providing all information about the CLA, I can pull as soon as tests pass. Should be ready soon.

@arielshaqed
Copy link
Contributor

  • There is a linked issue, ignoring that dud check.
  • There is a label include-changelog, ignoring that dud check.
  • CLA is literally attached to that PR, ignoring that the CLA check doesn't appear to want to run (and is not a GitHub action so cannot do anything about it).

PULLING, thanks!

@chrisdonlan you can of course use this immediately with no need to wait for a release - the Dockerfile changes are not a part of any release artefact, and indeed should not change any of our release artefacts.

@arielshaqed arielshaqed merged commit b8da7b3 into treeverse:master Dec 19, 2024
68 of 72 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arielshaqed arielshaqed arielshaqed left review comments

Assignees
No one assigned
Labels
area/ci contributor include-changelog PR description should be included in next release changelog infrastructure build, deploy and release processes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add an ability to switch the build architecture from 'musl' to 'glibc'
3 participants
@chrisdonlan @CLAassistant @arielshaqed